Open and accept zero reserve channels#4428
Open and accept zero reserve channels#4428tankyleo wants to merge 12 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
ffa1657 to
5fa3a7c
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4428 +/- ##
==========================================
- Coverage 86.18% 86.15% -0.04%
==========================================
Files 160 160
Lines 107536 108043 +507
Branches 107536 108043 +507
==========================================
+ Hits 92680 93084 +404
- Misses 12231 12338 +107
+ Partials 2625 2621 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
TheBlueMatt
left a comment
There was a problem hiding this comment.
chanmon_consistency needs to be updated to have a 0-reserve channel or two (I believe we now have three channels between each pair of peers, so we can just do it on a subset of them, in fact we could have three separate channel types for better coverage).
| /// Creates a new outbound channel to the given remote node and with the given value. | ||
| /// | ||
| /// The only difference between this method and [`ChannelManager::create_channel`] is that this method sets | ||
| /// the reserve the counterparty must keep at all times in the channel to zero. This allows the counterparty to |
There was a problem hiding this comment.
nit: If that's the only difference let's say create_channel_to_trusted_peer_0_reserve? Nice to be explicit, imo.
lightning/src/ln/channel.rs
Outdated
|
|
||
| let channel_value_satoshis = | ||
| our_funding_contribution_sats.saturating_add(msg.common_fields.funding_satoshis); | ||
| // TODO(zero_reserve): support reading and writing the `disable_channel_reserve` field |
There was a problem hiding this comment.
Two questions. Shouldn't we check that if a channel has the 0-reserve feature bit and if it is fail if the user isn't accepting 0-reserve? Also why shouldn't we just set it now? I'm not sure we need to bother with a staging bit, really, honestly...
|
Needs rebase now :/ |
471ba8f to
253db4d
Compare
Let me know if you prefer I rebase first |
|
Feel free to go ahead and rebase and squash, yea. |
5fa3a7c to
43be438
Compare
|
Squash diff (do not click compare just above, I pushed the wrong branch, and later corrected it): |
43be438 to
7fde002
Compare
|
|
|
✅ Added second reviewer: @joostjager |
|
🔔 1st Reminder Hey @TheBlueMatt @joostjager! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @TheBlueMatt @joostjager! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @TheBlueMatt @joostjager! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @TheBlueMatt @carlaKC! This PR has been waiting for your review. |
carlaKC
left a comment
There was a problem hiding this comment.
First pass - haven't gone through the tests yet.
lightning/src/ln/channelmanager.rs
Outdated
| /// If it does not confirm before we decide to close the channel, or if the funding transaction | ||
| /// does not pay to the correct script the correct amount, *you will lose funds*. | ||
| /// | ||
| /// # Zero-reserve |
There was a problem hiding this comment.
Shouldn't we be setting the option_zero_reserve feature in lightning/bolts#1140?
There was a problem hiding this comment.
Discussed offline, I'll hold off on signaling for now pending further spec discussions
5fa3a7c to
6a49cf6
Compare
|
| counterparty_balance_before_fee_msat: u64, feerate_per_kw: u32, nondust_htlc_count: usize, | ||
| broadcaster_dust_limit_satoshis: u64, channel_type: &ChannelTypeFeatures, | ||
| ) -> bool { | ||
| let commit_tx_fee_sat = commit_tx_fee_sat(feerate_per_kw, nondust_htlc_count, channel_type); |
There was a problem hiding this comment.
We have this logic repeated in get_next_commitment_stats given we have the same block to calculate to-self/remote outputs.
There was a problem hiding this comment.
See the commit below, I created better helpers.
lightning/src/ln/msgs.rs
Outdated
| (0, shutdown_scriptpubkey, (option, encoding: (ScriptBuf, WithoutLength))), | ||
| (1, channel_type, option), | ||
| (2, require_confirmed_inputs, option), | ||
| (4, disable_channel_reserve, option), |
There was a problem hiding this comment.
lol why is this an even TLV :(
There was a problem hiding this comment.
Hmm if receiver silently ignores, the two parties disagree on what HTLCs they can send right ?
There was a problem hiding this comment.
Isn't it unidirectional? If A sends B disable_channel_reserve then B is allowed to send more HTLCs because A doesn't care if B has a reserve. B can ignore that and decide to send less.
There was a problem hiding this comment.
Discussed offline, see commit below where I make the TLV odd.
lightning/src/ln/channel.rs
Outdated
| } else { | ||
| if !script::is_bolt2_compliant(&script, their_features) { | ||
| return Err(ChannelError::close(format!( | ||
| "Peer is signaling upfront_shutdown but has provided an unacceptable scriptpubkey format: {}", |
There was a problem hiding this comment.
nit, if you feel so inspired: might be a good opportunity to inline the variables in the {} -
| "Peer is signaling upfront_shutdown but has provided an unacceptable scriptpubkey format: {}", | |
| "Peer is signaling upfront_shutdown but has provided an unacceptable scriptpubkey format: {script}" |
There was a problem hiding this comment.
See commit below, I add a separate commit
| let counterparty_selected_channel_reserve_satoshis = get_v2_channel_reserve_satoshis( | ||
| post_channel_value, | ||
| MIN_CHAN_DUST_LIMIT_SATOSHIS, | ||
| prev_funding |
There was a problem hiding this comment.
Hmm, does this need a spec change/feature? If a peer gave us zero reserve prior to a splice and then we spliced, I'd expect it to stay zero, but I imagine the splice spec doesn't say that and theoretically this could cause us to have issues.
There was a problem hiding this comment.
Yes ! Now that splicing is merged, I'll propose an addition to the spec once this PR lands.
| debug_assert!(peer_state.is_connected); | ||
| peer_state.pending_msg_events.push(send_msg_err_event); | ||
| let err_str = "Please use accept_inbound_channel_from_trusted_peer_0conf to accept channels with zero confirmations.".to_owned(); | ||
| let err_str = "Please use accept_inbound_channel_from_trusted_peer to accept channels with zero confirmations.".to_owned(); |
There was a problem hiding this comment.
Misleading error message: If a user calls accept_inbound_channel_from_trusted_peer(id, node, 0, TrustedChannelFeatures::ZeroReserve, None) on a channel whose type requires zero-conf, they hit this error saying "Please use accept_inbound_channel_from_trusted_peer..." — but they already ARE calling that method. The issue is they passed ZeroReserve instead of ZeroConf or ZeroConfZeroReserve.
Consider updating the message to indicate the actual problem, e.g.:
"Channel requires zero confirmations. Please use TrustedChannelFeatures::ZeroConf or ZeroConfZeroReserve when calling accept_inbound_channel_from_trusted_peer."
There was a problem hiding this comment.
I prefer to leave as is
71a772e to
bbe7a62
Compare
|
🔔 1st Reminder Hey @TheBlueMatt @carlaKC! This PR has been waiting for your review. |
|
I think the changes here LGTM. Feel free to squash from my side, once @carlaKC says good I think we're good. |
The floor for *our* selected reserve is *their* dust limit.
The goal is to prevent any commitments with no outputs, since these are not broadcastable.
bbe7a62 to
91565f1
Compare
There is no need for this sentinel in `FundingScope` serialization code as this would only apply to pending `FundingScope`'s. Also, if the current scope has some zero-reserve, that reserve is carried over to all pending scopes automatically. Therefore it is not possible for a pending scope to have some 0-reserve without the current one also having it.
This new flag sets 0-reserve for the channel opener.
This new method sets 0-reserve for the channel accepter.
Co-Authored-By: HAL 9000
We do not care if our balance drops below the counterparty-selected reserve upon an inbound `update_add_htlc`. This is the counterparty's problem. Hence, we drop the assumption that once our balance rises above the counterparty-selected reserve, it will always remain above this reserve for the lifetime of a funding scope. In the following commit, we make the assumption that the counterparty does not complain if we push them below our selected reserve when adding a HTLC, so we accommodate this assumption here.
Note that this currently does not match the spec as we use an odd TLV for the `disable_channel_reserve` field in `open_channel2` and `accept_channel2` msgs. If the counterparty does not understand this field, that's ok as it just means that the counterparty will not send some HTLCs we would have accepted. We make the assumption that the counterparty will not complain if we send a HTLC that pushes their balance below our selected reserve; this could happen if the counterparty is the funder of the channel. They should not complain because if we push them below our selected reserve, this is our problem.
`ChannelContext::do_accept_channel_checks`, `ChannelContext::new_for_outbound_channel`, `ChannelContext::new_for_inbound_channel`, `InboundV1Channel::new`, `OutboundV1Channel::new`.
Reduce line count and indentation
Convert format string arguments to inline `{var}` captures where
the argument is a simple identifier (variable or constant). Field
accesses, method calls, and expressions remain as positional args.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
91565f1 to
2967e6f
Compare
Fixes #1801